Skip to content

Include reasoning for awards - #9101

Open
mjpost wants to merge 28 commits into
masterfrom
acl-awards-reasoning
Open

Include reasoning for awards#9101
mjpost wants to merge 28 commits into
masterfrom
acl-awards-reasoning

Conversation

@mjpost

@mjpost mjpost commented Jul 10, 2026

Copy link
Copy Markdown
Member

This PR adds the chair-provided reasoning for awards as an option. The schema is backwards-compatible but it requires changes to the library.

@mjpost mjpost added this to the 2026Q3 milestone Jul 10, 2026
@mjpost
mjpost requested a review from mbollmann July 10, 2026 19:57
@nschneid

Copy link
Copy Markdown
Collaborator

I wouldn't include the reasoning on hover

image

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.99%. Comparing base (1e6617f) to head (4c3d634).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9101      +/-   ##
==========================================
+ Coverage   96.95%   96.99%   +0.04%     
==========================================
  Files          36       36              
  Lines        3710     3732      +22     
==========================================
+ Hits         3597     3620      +23     
+ Misses        113      112       -1     
Files with missing lines Coverage Δ
python/acl_anthology/collections/__init__.py 100.00% <100.00%> (ø)
python/acl_anthology/collections/paper.py 94.88% <100.00%> (+0.26%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mjpost mjpost mentioned this pull request Jul 20, 2026
@mbollmann

Copy link
Copy Markdown
Member

I don’t like the distinction (in both the XML and the code) between "new" and "legacy" award tags. This could be much simplified. If we expect the reasoning to just be a single line of text, why not do:

  • <award reasoning="...">Best Paper Award</award>
  • Change the code the instantiate an Award class always, getting rid of the constant checks for str | Award (this seems unnecessarily complicated here).

@mbollmann

Copy link
Copy Markdown
Member

Alternatively, if we expect the reasoning to be potentially too long and unwieldy for an attribute, I would consider doing a one-time change to our schema to flip it around:

  • Without reasoning: <award name="Best Paper Award" />
  • With reasoning: <award name="Best Paper Award">Seminal paper that will change the field.</award>

@mjpost

mjpost commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

Mainly I was trying to make the code backward-compatible, but if you're fine with ditching that, I agree that a single tag structure is simpler.

The explanations this were were all paragraphs, which seemed unwieldy to me for an attribute, even though it is cleaner in the XML.
image

Does it make sense for the award name to be the attribute, and the explanation the text?

<award name="Best Paper" />
<award name="ACL 2026 Test of Time">After long deliberation, this paper was chosen because...</award>

@mjpost

mjpost commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

Alternatively, if we expect the reasoning to be potentially too long and unwieldy for an attribute, I would consider doing a one-time change to our schema to flip it around:

Okay, looks like we had the same idea. I'll go with that approach.

@mbollmann

Copy link
Copy Markdown
Member

Mainly I was trying to make the code backward-compatible,

Anything that changes the XML schema will break backward-compatibility anyway.

@mbollmann

Copy link
Copy Markdown
Member

Also, the interface wouldn’t have to change, paper.awards = ("Best Paper Award",) could still work even if they internally get converted to Award objects.

}*
& element award {
attribute name { text },
text

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MarkupText?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would require a bit of refactoring to support this; how extensive do we expect the "reasoning" to be?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we just support the paragraph of text we've currently encountered, at least for the moment.

@nschneid nschneid Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not allow any text formatting or linked URLs. Maybe it's not needed, just thought it could make conceptual sense.

Markup is also not supported for revision notes, but we see revision notes with line breaks for paragraphs or bulleted lists.

@mbollmann mbollmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost ready to merge unless we do want to add MarkupText for the reasoning; I’ll let you decide if that’s worth it.

Comment thread hugo/layouts/partials/award_icon.html Outdated
Comment thread hugo/layouts/papers/single.html Outdated
Comment thread hugo/layouts/partials/award_icon.html Outdated
}*
& element award {
attribute name { text },
text

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would require a bit of refactoring to support this; how extensive do we expect the "reasoning" to be?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants